Skip to content

feat: add stats/incr/nanmax#5989

Merged
Planeshifter merged 15 commits into
stdlib-js:developfrom
jalajk3004:nanmax
Jul 7, 2026
Merged

feat: add stats/incr/nanmax#5989
Planeshifter merged 15 commits into
stdlib-js:developfrom
jalajk3004:nanmax

Conversation

@jalajk3004

@jalajk3004 jalajk3004 commented Mar 12, 2025

Copy link
Copy Markdown
Contributor

Resolves #5550

Description

What is the purpose of this pull request?

This pull request:

  • adds a package nanmax, which works like max in which it calculates max incrementally ignoring NaN values

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@stdlib-bot stdlib-bot added Statistics Issue or pull request related to statistical functionality. Needs Review A pull request which needs code review. Good First PR A pull request resolving a Good First Issue. labels Mar 12, 2025
@stdlib-bot

stdlib-bot commented Mar 12, 2025

Copy link
Copy Markdown
Contributor

Coverage Report

Package Statements Branches Functions Lines
stats/incr/nanmax $\\color{green}137/137$
$\\color{green}+0.00\\%$
$\\color{green}8/8$
$\\color{green}+0.00\\%$
$\\color{green}2/2$
$\\color{green}+0.00\\%$
$\\color{green}137/137$
$\\color{green}+0.00\\%$

The above coverage report was generated for the changes in this PR.

@kgryte kgryte changed the title feat: add stats/incr/nancv feat: add stats/incr/nanmax Mar 12, 2025
@kgryte kgryte self-requested a review March 12, 2025 06:49
@jalajk3004

Copy link
Copy Markdown
Contributor Author

@kgryte, Removed all the linting errors, Please review this PR once again.

@kgryte kgryte added the Feature Issue or pull request for adding a new feature. label Apr 30, 2025
@kgryte

kgryte commented Nov 19, 2025

Copy link
Copy Markdown
Member

/stdlib merge

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Nov 19, 2025
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Nov 19, 2025
@kgryte

kgryte commented Dec 27, 2025

Copy link
Copy Markdown
Member

/stdlib merge

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Dec 27, 2025
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Dec 27, 2025
{
"name": "@stdlib/stats/incr/nanmax",
"version": "0.0.0",
"description": "Compute a maximum value incrementally, while ignoring `NaN` values.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No backticks around NaN in package.json descriptions.

Suggested change
"description": "Compute a maximum value incrementally, while ignoring `NaN` values.",
"description": "Compute a maximum value incrementally, while ignoring NaN values.",

*
* ## Notes
*
* - If provided `NaN` or a value which, when used in computations, results in `NaN`, the accumulated value is `NaN` for all future invocations.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note is copied from incrmax and contradicts the NaN-ignoring behavior of this package.

Suggested change
* - If provided `NaN` or a value which, when used in computations, results in `NaN`, the accumulated value is `NaN` for all future invocations.
* - If provided `NaN`, the value is ignored and the accumulated value remains unchanged.

v = randu() * 100.0;
}
max = accumulator( v );
console.log( '%d\t%d', v.toFixed( 4 ), ( max === null ) ? NaN : max.toFixed( 4 ) );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.toFixed() returns a string, so %d will coerce it back to a number and strip trailing zeros. Use %s instead.

Suggested change
console.log( '%d\t%d', v.toFixed( 4 ), ( max === null ) ? NaN : max.toFixed( 4 ) );
console.log( '%s\t%s', v.toFixed( 4 ), ( max === null ) ? 'null' : max.toFixed( 4 ) );

@Planeshifter Planeshifter added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Mar 10, 2026
@Planeshifter Planeshifter requested a review from a team July 7, 2026 04:48
@Planeshifter Planeshifter dismissed their stale review July 7, 2026 04:48

Requested changes applied in the maintainer cleanup commit: removed the incorrect NaN note in index.d.ts, dropped the package.json description backticks, and fixed the example output. Dismissing the stale review.

@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Jul 7, 2026
@Planeshifter Planeshifter force-pushed the nanmax branch 2 times, most recently from a0ac52c to 633fac5 Compare July 7, 2026 05:02
- Remove incorrect NaN-propagation note from index.d.ts (contradicted the NaN-ignoring behavior)
- Drop backticks from package.json description and align README/repl wording
- Remove inappropriate overflow note (a maximum does not accumulate/sum)
- Fix examples output to use %s and guard against a null accumulated value
- Remove a stray tab in a test title; add an all-NaN returns-null test
- Bump copyright year to 2026

@Planeshifter Planeshifter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's land it!

@Planeshifter Planeshifter added the Ready To Merge A pull request which is ready to be merged. label Jul 7, 2026
@stdlib-bot stdlib-bot removed Needs Review A pull request which needs code review. Needs Changes Pull request which needs changes before being merged. labels Jul 7, 2026
@stdlib-bot

Copy link
Copy Markdown
Contributor

PR Commit Message

feat: add `stats/incr/nanmax`

PR-URL: https://github.com/stdlib-js/stdlib/pull/5989
Closes: https://github.com/stdlib-js/stdlib/issues/5550

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Co-authored-by: stdlib-bot <noreply@stdlib.io>
Reviewed-by: Philipp Burckhardt <pburckhardt@outlook.com>

Please review the above commit message and make any necessary adjustments.

@Planeshifter Planeshifter merged commit 9fc1615 into stdlib-js:develop Jul 7, 2026
32 checks passed
@stdlib-bot stdlib-bot removed the Ready To Merge A pull request which is ready to be merged. label Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Issue or pull request for adding a new feature. Good First PR A pull request resolving a Good First Issue. Statistics Issue or pull request related to statistical functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC]: add stats/incr/nanmax

4 participants